Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: file leak in read_excel #30096

Merged
merged 1 commit into from
Dec 6, 2019
Merged

BUG: file leak in read_excel #30096

merged 1 commit into from
Dec 6, 2019

Conversation

jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Dec 5, 2019 •
edited by jreback
Loading

I think this
closes #30031
closes #29514
we'll probably want to wait a few days to make it official.

To make the new test decorator useful in the CI we'll need to add psutil to the environment (unless someone knows of a stdlib way of doing the same check?)

One more note, the check in check_no_file_leaks depends on __del__ having been called, which in turn depends on the gc. I'm not sure, but that might induce another layer of flakiness. If so, I think that can be fixed by calling gc.collect() before doing the flist == flist2 check

@alimcmaster1
Copy link
Member

LGTM - if this works! thanks for the fix!

@jreback jreback added the IO Excel read_excel, to_excel label Dec 5, 2019
@jreback jreback added this to the 1.0 milestone Dec 5, 2019
@jreback
Copy link
Contributor

jreback commented Dec 5, 2019

great. maybe keep rebasing a few times this to see if it fails.

@jbrockmendel
Copy link
Member Author

maybe keep rebasing a few times this to see if it fails.

Seems like a more effective way of checking this against many runs would be to merge and see if the py36 failures go away?

@jreback
Copy link
Contributor

jreback commented Dec 6, 2019

maybe keep rebasing a few times this to see if it fails.

Seems like a more effective way of checking this against many runs would be to merge and see if the py36 failures go away?

sure

@jreback jreback merged commit 383c14a into pandas-dev:master Dec 6, 2019
@jbrockmendel jbrockmendel deleted the cifix6 branch December 6, 2019 03:13
@jbrockmendel jbrockmendel mentioned this pull request Dec 9, 2019
Merged
@WillAyd WillAyd mentioned this pull request Dec 11, 2019
Closed
This was referenced Dec 19, 2019
Closed
Open
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@jbrockmendel @proost
BUG: file leak in read_excel (pandas-dev#30096)
c982cc5
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@jbrockmendel @proost
BUG: file leak in read_excel (pandas-dev#30096)
4d82e5d
@jorisvandenbossche jorisvandenbossche mentioned this pull request Jan 31, 2020
Closed
This was referenced Dec 5, 2020
Closed
Merged
@jonashaag
Copy link
Contributor

@jbrockmendel xref #47656: Does this miss a gc.collect() before the leak check? Some leaks will go away with a GC run. In #47656 there are a bunch of boundmethod objects that form a cycle with their originator class (SAS7BDATReader).

@jonashaag jonashaag mentioned this pull request Aug 22, 2022
Merged
5 tasks
@jbrockmendel
Copy link
Member Author

That's plausible, but I'll need a lot more caffeine to say more than that. An extra better-safe-than-sorry gc.collect call wouldn't be the end of the world.

@jonashaag jonashaag mentioned this pull request Aug 28, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
IO Excel read_excel, to_excel
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

CI: Flakey Test Flakey Test in CI - Unclosed File Handle
4 participants
@jbrockmendel @alimcmaster1 @jreback @jonashaag